-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial implementation of a shared cache on S3 Express #1032
Conversation
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small question about read window, otherwise LGTM
@@ -728,11 +769,11 @@ where | |||
if let Some(file_mode) = args.file_mode { | |||
filesystem_config.file_mode = file_mode; | |||
} | |||
filesystem_config.storage_class = args.storage_class; | |||
filesystem_config.storage_class = args.storage_class.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: revert? (probably the idea was to reject express cache when the mounted bucket is also on express?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, the .clone()
is to avoid invalidating args
, which is required later on.
But you are right we should also consider the storage class when we add validation for the shared cache!
Description of change
Initial implementation with minimal testing. Only enabled using the new compile-time feature flag
express_cache
.Also introduces the
--cache-block-size
cli option to experiment with block sizes. Applies to both the existing disk cache and the new shared cache. Only available when building with theblock_size
feature flag.Does this change impact existing behavior?
No.
Does this change need a changelog entry in any of the crates?
No.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).